Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix]: Update Timesheet Functionality and Load All Projects #3413

Merged
merged 2 commits into from
Dec 9, 2024

Conversation

Innocent-Akim
Copy link
Contributor

@Innocent-Akim Innocent-Akim commented Dec 9, 2024

Description

Please include a summary of the changes and the related issue.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

Previous screenshots

Please add here videos or images of previous status

Current screenshots

Please add here videos or images of previous status

Summary by CodeRabbit

  • New Features

    • Introduced organization-based project management in various components, enhancing data retrieval for tasks and filters.
    • Added loading states and improved user feedback mechanisms in modals.
    • Enhanced localization capabilities in the Kanban component.
  • Bug Fixes

    • Corrected task status references in filters to ensure accurate counts.
  • Style

    • Improved visual presentation of task entries and modal components with updated styles and class adjustments.
  • Chores

    • Refined component structures for better readability and maintainability without altering core functionalities.

Copy link
Contributor

coderabbitai bot commented Dec 9, 2024

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 eslint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx

Oops! Something went wrong! :(

ESLint: 8.46.0

ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct.

The config "next/core-web-vitals" was referenced from the config file in "/apps/web/.eslintrc.json".

If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.

Walkthrough

The changes in this pull request involve modifications to multiple components within the timesheet management system. Key updates include the replacement of the useTeamTasks hook with useOrganizationProjects in the AddTaskModal and EditTaskModal, enhancing project data retrieval. UI improvements are made in components like CalendarView, CompactTimesheetComponent, and TimesheetCard, focusing on visual presentation and user experience. Additionally, the TimeSheetFilterPopover and FilterWithStatus components see updates for better data handling and type safety, while the useTimesheet hook enhances error handling and state management.

Changes

File Path Change Summary
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx Replaced useTeamTasks with useOrganizationProjects, removed activeTeam, introduced organizationProjects, updated projectItemsLists.
apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx Added loading prop, adjusted UI elements, improved font size for status display, modified class names for better styling.
apps/web/app/[locale]/timesheet/[memberId]/components/CompactTimesheetComponent.tsx Updated default className for ImageWithLoader, increased size of LoadingSpinner.
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx Replaced useTeamTasks with useOrganizationProjects, modified payload structure in handleUpdateSubmit, added loading state to submit button.
apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.tsx Updated data property type, corrected status reference from DENIED to REJECTED.
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx Added useOrganizationProjects, updated items prop for MultiSelect, modified itemToString function.
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetCard.tsx Updated class name for TaskNameInfoDisplay, improved visual presentation.
apps/web/app/[locale]/timesheet/[memberId]/page.tsx Added useOrganizationProjects, introduced React.useEffect to fetch organization projects on mount.
apps/web/app/hooks/features/useTimesheet.ts Updated updateTimesheet function logic, improved error handling in deleteTaskTimesheet, enhanced getTaskTimesheet formatting.
apps/web/lib/components/Kanban.tsx Integrated useTranslations for localization, replaced hardcoded strings with localized strings.

Suggested labels

feature, WEB, Timesheets, Improvement

Suggested reviewers

  • evereq

Poem

In the fields where tasks do bloom,
A modal now makes room,
With projects from the organization,
It brings a new foundation.
So hop along, let's celebrate,
For timesheets now look great! 🐇✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (23)
apps/web/lib/components/Kanban.tsx (4)

Line range hint 235-296: Complete the internationalization of all strings

While "CREATE_TASK" has been translated, other strings like "Collapse Column" and "Edit Status" remain hardcoded. This creates an inconsistent user experience when the language is changed.

Apply this diff to complete the translations:

- <div
    className="hover:font-medium p-1.5 text-sm cursor-pointer"
    onClick={() => toggleColumn(title, false)}
>
-   Collapse Column
+   {t('common.COLLAPSE_COLUMN')}
</div>
<div
    className="hover:font-medium p-1.5 text-sm cursor-pointer"
    onClick={editOpenModal}
>
-   Edit Status
+   {t('common.EDIT_STATUS')}
</div>

Line range hint 380-420: Maintain consistency in string translations

Similar to the previous component, some strings remain untranslated while others use the translation system. This inconsistency should be addressed.

Apply this diff to complete the translations:

<div
    className="hover:font-medium p-1.5 text-sm cursor-pointer"
    onClick={() => toggleColumn(title, true)}
>
-   Collapse Column
+   {t('common.COLLAPSE_COLUMN')}
</div>
<div 
    className="hover:font-medium p-1.5 text-sm cursor-pointer" 
    onClick={openModal}
>
-   Edit Status
+   {t('common.EDIT_STATUS')}
</div>

Line range hint 1-565: Consider architectural improvements for better maintainability

The current implementation could benefit from several architectural improvements:

  1. Extract style constants to a theme configuration
  2. Consider using a modal management system to reduce duplicate modal state logic
  3. Move common translation keys to a shared constant file

Here's a suggested approach:

  1. Create a theme configuration file:
// theme/kanban.ts
export const KANBAN_THEME = {
  columnWidth: '355px',
  defaultHeight: '320px',
  headerPadding: '15px 7px',
  borderRadius: 'rounded-lg',
  // ... other style values
};
  1. Create a shared translations constant file:
// constants/translations.ts
export const KANBAN_TRANSLATIONS = {
  CREATE_TASK: 'common.CREATE_TASK',
  COLLAPSE_COLUMN: 'common.COLLAPSE_COLUMN',
  EDIT_STATUS: 'common.EDIT_STATUS',
  // ... other translation keys
};
  1. Consider implementing a modal management hook:
// hooks/useModalManager.ts
export const useModalManager = () => {
  const [modals, setModals] = useState<Record<string, boolean>>({});
  
  const openModal = (modalId: string) => 
    setModals(prev => ({ ...prev, [modalId]: true }));
  
  const closeModal = (modalId: string) => 
    setModals(prev => ({ ...prev, [modalId]: false }));
    
  return { modals, openModal, closeModal };
};

Line range hint 1-565: Optimize performance with memoization

Several opportunities for performance optimization through memoization:

  1. Memoize the column height calculation
  2. Memoize the items array to prevent unnecessary re-renders
  3. Memoize callback functions passed as props

Consider applying these optimizations:

// 1. Memoize column height calculation
const getKanbanColumnHeight = useMemo(() => 
  (itemsLength: number, containerRef: RefObject<HTMLDivElement> | undefined): string => {
    // ... existing logic
  }, 
  [/* dependencies */]
);

// 2. Memoize items array
const memoizedItems = useMemo(() => items, [items]);

// 3. Memoize callbacks
const handleCreateTask = useCallback(() => {
  openModal();
}, [openModal]);
apps/web/app/[locale]/timesheet/[memberId]/page.tsx (2)

Line range hint 40-62: Consider enhancing error handling and loading state management

While the initialization of useOrganizationProjects and the useEffect setup are functionally correct, consider these improvements:

  1. Add error handling for the getOrganizationProjects call
  2. Track and handle the loading state
  3. Consider memoizing getOrganizationProjects if it changes frequently

Here's a suggested implementation:

- const { getOrganizationProjects } = useOrganizationProjects();
+ const { getOrganizationProjects, loading: projectsLoading, error: projectsError } = useOrganizationProjects();

  React.useEffect(() => {
-   getOrganizationProjects();
+   try {
+     getOrganizationProjects();
+   } catch (error) {
+     console.error('Failed to fetch organization projects:', error);
+     // Consider showing an error notification to the user
+   }
  }, [getOrganizationProjects]);

+ // Handle loading state
+ if (projectsLoading) {
+   // Consider showing a loading indicator
+ }

+ // Handle error state
+ if (projectsError) {
+   // Consider showing an error message to the user
+ }

Line range hint 65-77: Consider optimizing filtering performance for large datasets

The current filtering implementation, while correct, might face performance issues with large datasets due to nested filtering operations. Consider these optimizations:

  1. Index the data for faster searching
  2. Implement debounced search
  3. Consider pagination or virtual scrolling for large lists

Here's a suggested optimization:

+ const debouncedSearch = useDebounce(search, 300);
+ 
  const filterDataTimesheet = useMemo(() => {
+   if (!debouncedSearch) return timesheet;
+ 
+   const searchTerms = debouncedSearch.toLowerCase().split(' ');
    const filteredTimesheet =
      timesheet
        .filter((v) =>
          v.tasks.some(
            (task) => {
+             const searchableText = [
+               task.task?.title,
+               task.employee?.fullName,
+               task.project?.name
+             ].map(text => text?.toLowerCase() ?? '').join(' ');
+             
+             return searchTerms.every(term => searchableText.includes(term));
            }
          )
        );

    return filteredTimesheet;
-  }, [timesheet, lowerCaseSearch]);
+  }, [timesheet, debouncedSearch]);
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetCard.tsx (1)

Line range hint 1-192: Consider enhancing type safety, accessibility, and performance.

While the current implementation works well, here are some suggestions for improvement:

  1. Type Safety:

    • Add null checks when accessing data?.PENDING in the groupByDate call
    • Consider using TypeScript's non-null assertion operator more judiciously
  2. Accessibility:

    • Add aria-label to the accordion items to describe their content
    • Consider adding aria-expanded state to accordion triggers
  3. Performance:

    • Memoize the results of getStatusTimesheet and groupByDate using useMemo
    • Consider implementing React.memo for child components that receive stable props

Example implementation for performance optimization:

+ import { useMemo } from 'react';

export const TimesheetCardDetail = ({ data }: { data?: Record<TimesheetStatus, TimesheetLog[]> }) => {
    const { getStatusTimesheet, groupByDate } = useTimesheet({});
    const { timesheetGroupByDays } = useTimelogFilterOptions();
-   const timesheetGroupByDate = groupByDate(data?.PENDING || [])
+   const timesheetGroupByDate = useMemo(() => 
+       groupByDate(data?.PENDING || []),
+       [data?.PENDING, groupByDate]
+   );
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx (2)

100-102: Simplify the project selection implementation.

The current implementation of itemToString has unnecessary complexity and redundant checks.

Consider simplifying the code:

   <MultiSelect
     localStorageKey="timesheet-select-filter-projects"
     removeItems={shouldRemoveItems}
-    items={organizationProjects ?? []}
+    items={organizationProjects || []}
     itemToString={(project) =>
-      (organizationProjects && project ? project.name : '') || ''
+      project?.name || ''
     }
     itemId={(item) => item.id}
     onValueChange={(selectedItems) => setProjectState(selectedItems as any)}
     multiSelect={true}
     triggerClassName="dark:border-gray-700"
   />

Improvements:

  1. Simplified null check using logical OR
  2. Removed unnecessary organizationProjects check in itemToString
  3. Used optional chaining for cleaner null handling

Line range hint 21-22: Improve type safety by removing 'any' type assertions.

The state setters use type 'any' which reduces type safety. Consider defining proper types for the state values.

Example improvement:

interface TimelogFilterState {
  employee: Member[];
  project: Project[];
  task: Task[];
  statusState: StatusOption[];
}

const setProjectState = (selectedItems: Project[]) => {
  // Implementation
};
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx (3)

51-51: Improved state management and data handling

Good improvements in state management:

  1. Default value for isBillable with nullish coalescing
  2. Updated project data source to use organizationProjects
  3. Proper handling of undefined selectedValues

Consider adding type validation for the project data structure.

- selectedValues={selectedValues ?? []}
+ selectedValues={selectedValues ?? {} as Record<string, Item>}

Also applies to: 56-56, 234-234


310-318: Good UI/UX improvements with loading state

The loading state implementation is good, but consider these accessibility improvements:

 <button
   disabled={loadingUpdateTimesheet}
+  aria-busy={loadingUpdateTimesheet}
   type="submit"
   className={clsxm(
     'bg-primary dark:bg-primary-light h-[2.3rem] w-[5.5rem] justify-center font-normal flex items-center text-white px-2 rounded-lg',
+    loadingUpdateTimesheet && 'cursor-not-allowed opacity-70'
   )}>
   {loadingUpdateTimesheet && (
-    <ReloadIcon className="w-4 h-4 mr-2 animate-spin" />
+    <ReloadIcon className="w-4 h-4 mr-2 animate-spin" aria-hidden="true" />
   )}
-  {loadingUpdateTimesheet ? "Processing..." : t('common.SAVE')}
+  <span>{loadingUpdateTimesheet ? "Processing..." : t('common.SAVE')}</span>
 </button>

Line range hint 72-83: Consider using a form validation library

The current manual validation works but could be improved:

  1. Time format validation is basic
  2. Error messages are shown through alert()

Consider using a form validation library like react-hook-form with zod for better validation:

  • Type-safe validation
  • Better error handling
  • More maintainable validation rules
  • Improved user experience with inline error messages
apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx (2)

144-146: Consider consolidating duplicated styles

The TaskNameInfoDisplay styling is duplicated between the CalendarDataView and BaseCalendarDataView components. Consider extracting these common styles into a shared constant or utility function to maintain consistency and reduce duplication.

+ // Add at the top of the file
+ const taskNameDisplayStyles = {
+   container: 'rounded-sm h-auto !px-[0.3312rem] py-[0.2875rem] shadow-[0px_0px_15px_0px_#e2e8f0] dark:shadow-transparent',
+   title: 'text-sm text-ellipsis overflow-hidden !text-[#293241] dark:!text-white'
+ };

  // Then in both components:
  <TaskNameInfoDisplay
    task={task.task}
-   className={cn(
-     'rounded-sm h-auto !px-[0.3312rem] py-[0.2875rem] shadow-[0px_0px_15px_0px_#e2e8f0] dark:shadow-transparent'
-   )}
-   taskTitleClassName={cn(
-     'text-sm text-ellipsis overflow-hidden !text-[#293241] dark:!text-white'
-   )}
+   className={cn(taskNameDisplayStyles.container)}
+   taskTitleClassName={cn(taskNameDisplayStyles.title)}
    showSize={true}
    dash
    taskNumberClassName="text-sm"
  />

Also applies to: 238-241


Line range hint 22-63: Simplify render logic and improve loading state handling

The current implementation has nested ternary operators and doesn't effectively use the loading prop. Consider restructuring for better readability and loading state handling.

export function CalendarView({ data, loading, user }: { data?: GroupedTimesheet[], loading: boolean, user?: IUser | undefined }) {
    const t = useTranslations();
    const { timesheetGroupByDays } = useTimelogFilterOptions();
    const defaultDaysLabels = [/* ... */];

+   const renderContent = () => {
+     if (loading) {
+       return (
+         <div className="flex items-center justify-center h-full">
+           <p>{t('pages.timesheet.LOADING')}</p>
+         </div>
+       );
+     }
+
+     if (!data || data.length === 0) {
+       return (
+         <div className="flex items-center justify-center h-full min-h-[280px]">
+           <p>{t('pages.timesheet.NO_ENTRIES_FOUND')}</p>
+         </div>
+       );
+     }
+
+     const viewComponents = {
+       Monthly: MonthlyCalendarDataView,
+       Weekly: WeeklyCalendarDataView,
+     };
+
+     const ViewComponent = viewComponents[timesheetGroupByDays as keyof typeof viewComponents];
+     return ViewComponent ? (
+       <ViewComponent data={data} daysLabels={defaultDaysLabels} t={t} />
+     ) : (
+       <CalendarDataView data={data} t={t} />
+     );
+   };

    return (
        <div className="grow h-full bg-[#FFFFFF] dark:bg-dark--theme">
-           {data ? (
-               data.length > 0 ? (
-                   <>
-                       {timesheetGroupByDays === 'Monthly' ? (
-                           <MonthlyCalendarDataView data={data} daysLabels={defaultDaysLabels} t={t} />
-                       ) : timesheetGroupByDays === 'Weekly' ? (
-                           <WeeklyCalendarDataView data={data} daysLabels={defaultDaysLabels} t={t} />
-                       ) : (
-                           <CalendarDataView data={data} t={t} />
-                       )}
-                   </>
-               ) : (
-                   <div className="flex items-center justify-center h-full min-h-[280px]">
-                       <p>{t('pages.timesheet.NO_ENTRIES_FOUND')}</p>
-                   </div>
-               )
-           ) : (
-               <div className="flex items-center justify-center h-full">
-                   <p>{t('pages.timesheet.LOADING')}</p>
-               </div>
-           )}
+           {renderContent()}
        </div>
    );
}
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (2)

Line range hint 249-268: Add time zone handling and validate work hours

The time calculation logic has several potential issues:

  1. No time zone handling which could cause issues with international teams
  2. Missing validation for maximum work hours per shift
  3. Complex time calculation could be simplified

Consider these improvements:

 const calculateTotalHoursHour = React.useCallback(
     (start: string, end: string): string => {
         if (!start || !end) return '00:00h';
         const startMinutes = convertToMinutesHour(start);
         const endMinutes = convertToMinutesHour(end);
         const totalMinutes = endMinutes >= startMinutes
             ? endMinutes - startMinutes
             : 1440 - startMinutes + endMinutes;
+        // Add validation for maximum work hours (e.g., 24 hours)
+        if (totalMinutes > 1440) {
+            throw new Error('Shift duration cannot exceed 24 hours');
+        }
         const hours = Math.floor(totalMinutes / 60);
         const minutes = totalMinutes % 60;
         return `${String(hours).padStart(2, '0')}:${String(minutes).padStart(2, '0')}h`;
     },
     []
 );

Line range hint 171-181: Implement form submission and validation

The form is missing critical functionality:

  1. No form submission handler
  2. No validation for required fields before submission
  3. No loading states during submission

Consider implementing form handling:

+const [isSubmitting, setIsSubmitting] = React.useState(false);
+const [errors, setErrors] = React.useState<Record<string, string>>({});

+const validateForm = () => {
+    const newErrors: Record<string, string> = {};
+    if (!task) newErrors.task = 'Task is required';
+    if (!notes.trim()) newErrors.notes = 'Notes are required';
+    if (!selectedValues.Project) newErrors.project = 'Project is required';
+    setErrors(newErrors);
+    return Object.keys(newErrors).length === 0;
+};

+const handleSubmit = async () => {
+    if (!validateForm()) return;
+    setIsSubmitting(true);
+    try {
+        // Add your submission logic here
+        closeModal();
+    } catch (error) {
+        console.error('Failed to submit:', error);
+    } finally {
+        setIsSubmitting(false);
+    }
+};

 <button
     type="submit"
+    disabled={isSubmitting}
     className={clsxm(
         'bg-primary dark:bg-primary-light h-[2.3rem] w-[5.5rem] justify-center font-normal flex items-center text-white px-2 rounded-lg',
+        isSubmitting && 'opacity-50 cursor-not-allowed'
     )}
+    onClick={handleSubmit}
 >
-    {t('common.SAVE')}
+    {isSubmitting ? t('common.SAVING') : t('common.SAVE')}
 </button>
apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.tsx (5)

Line range hint 8-19: Fix inconsistent status naming and improve type safety

The component has inconsistencies in status naming and type safety issues:

  1. FilterStatus type uses 'Rejected' while the code accesses DENIED in the data
  2. String literal 'IN REVIEW' bypasses TypeScript's type checking

Consider applying these changes:

-export type FilterStatus = 'All Tasks' | 'Pending' | 'Approved' | 'In review' | 'Draft' | 'Rejected';
+export type FilterStatus = 'All Tasks' | 'Pending' | 'Approved' | 'In Review' | 'Draft' | 'Rejected';

export function FilterWithStatus({
	activeStatus,
	onToggle,
	className,
	data
}: Readonly<{
	activeStatus: FilterStatus;
-	data?: Record<TimesheetStatus, TimesheetLog[]>
+	data?: Partial<Record<TimesheetStatus, TimesheetLog[]>>
	onToggle: (status: FilterStatus) => void;
	className?: HTMLAttributes<HTMLDivElement>;
}>) {

Line range hint 35-42: Optimize memo dependencies and fix status access

The buttonData memo uses the translation function inside but doesn't include it in dependencies. Also, the status access needs to be aligned with TimesheetStatus type.

Apply these changes:

const buttonData = React.useMemo(() => {
	const counts = {
		[t('pages.timesheet.ALL_TASKS')]: Object.values(data ?? {}).reduce((total, tasks) => total + (tasks?.length ?? 0), 0),
		[t('pages.timesheet.PENDING')]: data?.PENDING?.length ?? 0,
		[t('pages.timesheet.APPROVED')]: data?.APPROVED?.length ?? 0,
-		[t('pages.timesheet.IN_REVIEW')]: data?.['IN REVIEW']?.length ?? 0,
+		[t('pages.timesheet.IN_REVIEW')]: data?.IN_REVIEW?.length ?? 0,
		[t('pages.timesheet.DRAFT')]: data?.DRAFT?.length ?? 0,
-		[t('pages.timesheet.REJECTED')]: data?.DENIED?.length ?? 0,
+		[t('pages.timesheet.REJECTED')]: data?.REJECTED?.length ?? 0,
	};
	return Object.entries(counts).map(([label, count]) => ({
		label: label as FilterStatus,
		count,
		icon: <i className={statusIcons[label as FilterStatus]} />,
	}));
-}, [data]);
+}, [data, t]);

Line range hint 55-77: Improve accessibility and button rendering

The current implementation has accessibility issues and uses index as key which is an anti-pattern in React.

Consider these improvements:

 {buttonData.map(({ label, count, icon }, index) => (
 	<Button
-		key={index}
+		key={label}
 		className={clsxm(
 			'group flex items-center justify-start h-[2.2rem] rounded-xl w-full',
 			'dark:bg-gray-800 dark:border-primary-light bg-transparent text-[#71717A] w-[80px]',
 			activeStatus === label &&
 			'text-primary bg-white shadow-2xl dark:text-primary-light font-bold  border'
 		)}
 		onClick={() => onToggle(label)}
+		aria-label={`Filter by ${label.toLowerCase()} status. ${count} items`}
+		aria-pressed={activeStatus === label}
 	>

Line range hint 21-32: Consider using TypeScript enum or const assertions for status icons

The status icons mapping could benefit from stronger typing to prevent potential mismatches.

Consider this improvement:

-const statusIcons: Record<FilterStatus, string> = {
+const statusIcons = {
	'All Tasks': 'icon-all',
	Pending: 'icon-pending',
	Approved: 'icon-approved',
	'In review': 'icon-rejected',
	Draft: 'icon-approved',
	Rejected: 'icon-rejected',
-};
+} as const;

Status naming inconsistencies found in the timesheet implementation

There are several inconsistencies in how timesheet statuses are handled across the codebase:

  • DENIED vs REJECTED: The backend uses DENIED but the UI displays it as REJECTED
  • IN REVIEW vs IN_REVIEW: Mixed usage of space and underscore in the status name
  • The FilterStatus type uses display-friendly names ('In review') while the data object uses uppercase keys ('IN REVIEW')
  • Inconsistent casing: Some components use lowercase for valid statuses (e.g., in timer/timesheet/status/route.ts)

Recommendations:

  • Standardize on either DENIED or REJECTED across the entire codebase
  • Use consistent casing (preferably uppercase) for status values
  • Standardize the format of multi-word statuses (either with space or underscore)
  • Consider using the enum TimesheetStatus consistently instead of string literals
🔗 Analysis chain

Line range hint 1-77: Verify status naming consistency across the codebase

Let's ensure that the status naming changes are consistent across all components.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for inconsistent status naming across the codebase

# Check for different variations of status names
echo "Checking for status name variations..."
rg -i "denied|rejected" --type tsx --type ts

# Check TimesheetStatus enum definition
echo -e "\nChecking TimesheetStatus definition..."
rg "enum\s+TimesheetStatus" --type ts

# Check for 'IN REVIEW' vs 'IN_REVIEW' usage
echo -e "\nChecking review status variations..."
rg -i "('IN[\s_]REVIEW'|\"IN[\s_]REVIEW\")" --type tsx --type ts

Length of output: 482


Script:

#!/bin/bash
# Let's try a different approach without file type restrictions

# Check for TimesheetStatus enum definition
echo "Checking TimesheetStatus enum definition..."
ast-grep --pattern 'enum TimesheetStatus {
  $$$
}'

# Check for status name variations in the codebase
echo -e "\nChecking status name variations..."
rg -i "status.*=.*('DENIED'|'REJECTED'|'IN REVIEW'|'IN_REVIEW')"

# Check for data access patterns
echo -e "\nChecking data access patterns..."
rg "data\?\.(DENIED|REJECTED|IN_REVIEW|'IN REVIEW'|DRAFT|PENDING|APPROVED)"

# Check for translations related to these statuses
echo -e "\nChecking translation keys..."
rg "'pages\.timesheet\.(DENIED|REJECTED|IN_REVIEW|DRAFT|PENDING|APPROVED)'"

Length of output: 4188

apps/web/app/hooks/features/useTimesheet.ts (2)

Line range hint 165-176: Improve error handling consistency across async operations

The error handling patterns are inconsistent across the async operations:

  • updateTimesheet throws the error
  • createTimesheet swallows the error
  • updateTimesheetStatus logs the error but continues silently

This inconsistency could make debugging difficult and lead to silent failures.

Consider standardizing error handling:

 const createTimesheet = useCallback(
     async ({ ...timesheetParams }: UpdateTimesheet) => {
         if (!user) {
             throw new Error("User not authenticated");
         }
         try {
             const response = await queryCreateTimesheet(timesheetParams);
             setTimesheet((prevTimesheet) => [
                 response.data,
                 ...(prevTimesheet || [])
             ]);
+        } catch (error) {
-            console.error('Error:', error);
+            console.error('Error creating timesheet:', error);
+            throw error;
         }
     },
     [queryCreateTimesheet, setTimesheet, user]
 );

 const updateTimesheetStatus = useCallback(
     async ({ status, ids }: { status: TimesheetStatus; ids: ID[] | ID }) => {
-        if (!user) return;
+        if (!user) {
+            throw new Error("User not authenticated");
+        }
         const idsArray = Array.isArray(ids) ? ids : [ids];
         try {
             // ... existing code ...
         } catch (error) {
             console.error('Error updating timesheet status:', error);
+            throw error;
         }
     },
     [queryUpdateTimesheetStatus, setTimesheet, user]
 );

Also applies to: 246-259, 277-289


Line range hint 224-244: Optimize performance of status mapping and grouping operations

The status mapping and grouping operations could be optimized to prevent unnecessary recalculations:

  1. STATUS_MAP is recreated on every call to getStatusTimesheet
  2. The grouping functions aren't memoized

Consider these optimizations:

+const STATUS_MAP: Record<TimesheetStatus, TimesheetLog[]> = {
+    PENDING: [],
+    APPROVED: [],
+    DENIED: [],
+    DRAFT: [],
+    'IN REVIEW': []
+};

 const getStatusTimesheet = (items: TimesheetLog[] = []) => {
-    const STATUS_MAP: Record<TimesheetStatus, TimesheetLog[]> = {
-        PENDING: [],
-        APPROVED: [],
-        DENIED: [],
-        DRAFT: [],
-        'IN REVIEW': []
-    };
+    const result = JSON.parse(JSON.stringify(STATUS_MAP));

     return items.reduce((acc, item) => {
         const status = item.timesheet.status;
         if (isTimesheetStatus(status)) {
             acc[status].push(item);
         } else {
             console.warn(`Invalid timesheet status: ${status}`);
         }
         return acc;
-    }, STATUS_MAP);
+    }, result);
 }

+const memoizedGroupByDate = useMemo(() => groupByDate, []);
+const memoizedGroupByWeek = useMemo(() => groupByWeek, []);
+const memoizedGroupByMonth = useMemo(() => groupByMonth, []);

Also applies to: 268-276

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 34fe05d and a14b935.

📒 Files selected for processing (10)
  • apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (2 hunks)
  • apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx (3 hunks)
  • apps/web/app/[locale]/timesheet/[memberId]/components/CompactTimesheetComponent.tsx (1 hunks)
  • apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx (7 hunks)
  • apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.tsx (1 hunks)
  • apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx (3 hunks)
  • apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetCard.tsx (1 hunks)
  • apps/web/app/[locale]/timesheet/[memberId]/page.tsx (3 hunks)
  • apps/web/app/hooks/features/useTimesheet.ts (1 hunks)
  • apps/web/lib/components/Kanban.tsx (4 hunks)
🔇 Additional comments (9)
apps/web/app/[locale]/timesheet/[memberId]/components/CompactTimesheetComponent.tsx (1)

33-33: LGTM: Improved loading spinner proportions

The increased spinner size (w-5/h-5) provides better visual balance within its container (w-6/h-6).

apps/web/app/[locale]/timesheet/[memberId]/page.tsx (2)

11-11: LGTM: Clean import addition

The useOrganizationProjects hook import is properly grouped with other custom hooks.


Line range hint 1-1: Verify complete migration from useTeamTasks

Let's verify that all instances of useTeamTasks have been replaced with useOrganizationProjects across the codebase.

apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetCard.tsx (1)

154-154: LGTM! Consider verifying responsive behavior.

The styling update aligns with the broader UI consistency improvements across the timesheet components. The change from shadow effects to rounded corners with specific padding maintains visual hierarchy while potentially improving readability.

Please verify the visual appearance of task names across different screen sizes and text lengths. Run the following script to find related components that might need similar styling updates:

✅ Verification successful

Style update looks good and consistent with component usage

The styling change to TaskNameInfoDisplay is appropriate as it's used within a timesheet task row where consistent spacing and visual hierarchy are important. The component is rendered with proper padding and rounded corners, maintaining visual consistency with the parent accordion item's design. The shadow effect modification (shadow-[0px_0px_15px_0px_#e2e8f0] dark:shadow-transparent) provides subtle depth in light mode while staying flat in dark mode, which is consistent with modern UI practices.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other TaskNameInfoDisplay usages to ensure consistent styling
rg "TaskNameInfoDisplay.*className" --type tsx

Length of output: 76


Script:

#!/bin/bash
# Search for TaskNameInfoDisplay component usage with broader file patterns
rg "TaskNameInfoDisplay.*className" --type ts --type tsx --type jsx --type js

# Find all files containing TaskNameInfoDisplay to understand its usage
rg "TaskNameInfoDisplay" --type ts --type tsx --type jsx --type js

Length of output: 202


Script:

#!/bin/bash
# First, find all TypeScript/React files that might contain TaskNameInfoDisplay
fd "\.(tsx|ts|jsx|js)$"

# Then search for TaskNameInfoDisplay in all files
rg "TaskNameInfoDisplay"

Length of output: 71167


Script:

#!/bin/bash
# Find the file content to understand TaskNameInfoDisplay usage
fd "TimesheetCard.tsx" | xargs cat

Length of output: 9985

apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx (1)

2-2: LGTM! Verify hook usage across the application.

The addition of useOrganizationProjects hook aligns with the PR objective to load all projects and maintains consistency with other components.

Let's verify the consistent usage of this hook across the application:

Also applies to: 16-16

✅ Verification successful

Based on the search results, I can see that the useOrganizationProjects hook is consistently used across multiple components in the application, including:

  1. TimeSheetFilterPopover.tsx
  2. AddTaskModal.tsx
  3. EditTaskModal.tsx
  4. task-all-status-type.tsx
  5. create-project-modal.tsx
  6. integration-setting.tsx
  7. page.tsx (timesheet)

Let's verify the hook's implementation and its usage pattern:


Based on the search results, I can now provide a definitive response about the hook usage:

✓ The hook usage is consistent and correct across the application

The useOrganizationProjects hook is properly used across multiple components with consistent patterns:

  • Components that need project data use organizationProjects for listing/filtering
  • Components that need to modify projects use specific actions like editOrganizationProject, createOrganizationProject
  • Components that need to look up projects use getOrganizationProject

The hook's implementation in useOrganizationProjects.ts follows standard patterns with proper state management using atoms.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistent usage of useOrganizationProjects hook
# Expected: Should find usage in AddTaskModal, EditTaskModal, and page.tsx as mentioned in the AI summary

# Search for useOrganizationProjects usage
rg "useOrganizationProjects" --type tsx

Length of output: 67


Script:

#!/bin/bash
# Let's try with a more generic search approach to find TypeScript/React files
# Search for useOrganizationProjects usage across ts and tsx files
rg "useOrganizationProjects" -t ts -t tsx

# Also search for the hook definition
rg "export.*useOrganizationProjects" -t ts -t tsx

Length of output: 149


Script:

#!/bin/bash
# Search for useOrganizationProjects usage without file type restrictions
rg "useOrganizationProjects"

# Let's also find all files with .ts or .tsx extension to understand the structure
fd "\.(ts|tsx)$"

Length of output: 67872


Script:

#!/bin/bash
# Check the hook implementation
rg -A 5 "export function useOrganizationProjects" 

# Check how the hook is used in these components
rg -A 2 "const.*useOrganizationProjects" 

Length of output: 3307

apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx (1)

9-9: Verify the impact of switching from useTeamTasks to useOrganizationProjects

The switch to useOrganizationProjects aligns with the PR objectives to load all projects. However, we should verify that this change doesn't affect existing functionality.

Also applies to: 25-25

apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx (1)

105-108: Verify status terminology change across the codebase

The status "DENIED" is being replaced with "REJECTED". While this change improves clarity, we should ensure this terminology update is consistent across the entire application.

✅ Verification successful

Status terminology change is part of a broader standardization effort

The change from 'DENIED' to 'REJECTED' appears to be part of a systematic update across the application. The search results show:

  • Multiple components are already using the 'DENIED' to 'REJECTED' conversion pattern
  • Both terms exist in the codebase, with 'REJECTED' being used in newer components and translations
  • The change is being handled consistently with conditional rendering: status === 'DENIED' ? 'REJECTED' : status
  • Translation files in multiple languages support both terms

This change appears to be a deliberate standardization effort, and the current implementation correctly handles the transition.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining instances of 'DENIED' status
rg -i "('|\")DENIED('|\")" --type typescript --type javascript

# Search for current usage of 'REJECTED' status
rg -i "('|\")REJECTED('|\")" --type typescript --type javascript

Length of output: 209


Script:

#!/bin/bash
# Search for any remaining instances of 'DENIED' status using ripgrep
rg -i "('|\")DENIED('|\")" --type ts --type tsx --type js --type jsx

# Search for current usage of 'REJECTED' status
rg -i "('|\")REJECTED('|\")" --type ts --type tsx --type js --type jsx

Length of output: 207


Script:

#!/bin/bash
# Search for any remaining instances of 'DENIED' status using ripgrep without file type filters
rg -i "('|\")DENIED('|\")"

# Search for current usage of 'REJECTED' status
rg -i "('|\")REJECTED('|\")"

Length of output: 10904

apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (1)

2-2: Verify impact of switching from team-based to organization-based projects

The change from useTeamTasks to useOrganizationProjects represents a significant architectural shift in how projects are loaded. While this aligns with the PR objective, we should ensure this change doesn't affect existing team-specific functionality.

Also applies to: 20-20

✅ Verification successful

No impact from switching to organization-based projects

The change from useTeamTasks to useOrganizationProjects in AddTaskModal.tsx is safe. The useOrganizationProjects hook provides organization-wide project data through proper API calls and state management, while maintaining backward compatibility. The hook's implementation shows it handles project data at the organization level without breaking existing functionality.

Key findings:

  • useOrganizationProjects properly manages organization projects through tenant-aware API calls
  • No direct dependencies on team-specific functionality in the implementation
  • The change aligns with the codebase's architecture where projects are managed at organization level
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any remaining team-specific project references
rg "team.*project" --type typescript

# Check for any components still using useTeamTasks
rg "useTeamTasks" --type typescript

Length of output: 144


Script:

#!/bin/bash
# Check for any remaining team-specific project references
rg "team.*project"

# Check for any components still using useTeamTasks
rg "useTeamTasks"

# Check the implementation of useOrganizationProjects to understand the change
ast-grep --pattern 'export function useOrganizationProjects() { $$$ }'

# Check if there are any other components using useOrganizationProjects
rg "useOrganizationProjects"

Length of output: 34727

apps/web/app/hooks/features/useTimesheet.ts (1)

170-173: Verify the timesheet structure change across the codebase

The change from item.timesheet.id to item.id suggests a structural modification in how timesheets are identified. This could be a breaking change if other components still expect the old nested structure.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx (2)

310-318: Enhance loading state UX

Good addition of loading state, but consider these improvements:

  1. Disable all interactive elements during submission
  2. Show loading indicator on the entire modal
 <form onSubmit={handleUpdateSubmit} className="flex flex-col w-full">
+  <div className={clsxm("relative", loadingUpdateTimesheet && "pointer-events-none opacity-70")}>
   {/* ... existing form content ... */}
+  </div>
 </form>

163-164: Enhance accessibility for task information display

Consider adding these accessibility improvements:

  1. Add aria-label to task information section
  2. Add screen reader text for task status
 <TaskNameInfoDisplay
   task={dataTimesheet.task}
+  aria-label={`Task: ${dataTimesheet.task?.title}`}
   className={clsxm('rounded-sm h-auto !px-[0.3312rem] py-[0.2875rem] shadow-[0px_0px_15px_0px_#e2e8f0] dark:shadow-transparent')}
   taskTitleClassName={clsxm('text-sm text-ellipsis overflow-hidden')}
   showSize={true}
   dash
   taskNumberClassName="text-sm"
 />

Also applies to: 232-232

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between a14b935 and 239ed66.

📒 Files selected for processing (1)
  • apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx (7 hunks)
🔇 Additional comments (3)
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx (3)

9-9: LGTM: Import changes enhance error handling and UX

The replacement of team-based hooks with organization-level hooks and addition of toast notifications improve the component's functionality.

Also applies to: 15-17


51-51: LGTM: Improved state initialization and data handling

Good improvements:

  1. Proper fallback for isBillable with nullish coalescing
  2. Consistent use of organizationProjects
  3. Proper initialization of selectedValues with project data

Also applies to: 56-56, 65-65


99-132: Improve error handling and modal behavior

While the toast notifications are a good addition, there are still some issues that need to be addressed:

  1. Raw error exposure in the error message
  2. Modal closes on both success and error states

The previous review comment's suggestions are still valid and should be implemented:

  1. Use a user-friendly error message
  2. Keep the modal open on error to allow retry

@Innocent-Akim Innocent-Akim self-assigned this Dec 9, 2024
@Innocent-Akim Innocent-Akim added feature Ever Teams Bug fix Don't Merge Do not merge this PR until label not removed Improvement Improvement and removed Don't Merge Do not merge this PR until label not removed labels Dec 9, 2024
@evereq evereq merged commit 31507e2 into develop Dec 9, 2024
13 checks passed
@evereq evereq deleted the feat/update-timesheet-load-projects branch December 9, 2024 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants